Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split serial #1954

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Split serial #1954

wants to merge 1 commit into from

Conversation

M1cha
Copy link

@M1cha M1cha commented Oct 5, 2023

This picks up on #1117. I created this as a draft PR so I can collect feedback of the maintainers before investing the work to finish things up. Specifically:

  • Are you okay with COBS?
  • Do you agree with the general way the code works?

known TODOs:

  • Add support for sending with an interrupt-based UART driver.
  • Select transport via device tree.
  • Implement and test sensors.

It's also very apparent, that I had to copy quite some code from the BLE central. In future we might want to refactor both implementations with the goal of changing the abstraction layer to something that reduces code duplication even more. Right now we should focus on getting this working though so we can see how much overlap we really end up with.

@petejohanson
Copy link
Contributor

I'll review COBS a bit closer, but generally I don't see a huge problem using something like this for this. We don't really need advanced features like protobuf for things like this, e.g. backwards/forwards compat with encoded data, etc.

The general approach: I agree, there's definitely some duplication. I'd love our final solution to abstract out just the "transport" portion into an API that allows swapping out the implementation of just that piece, instead of duplicating the whole thing, but I understand the hesitancy to do this "too early". I think as long as we have an eye on this eventual de-dup, I'm fine with it.

Now that upstream Zephyr in recent versions has added a PIO based UART driver, I think sticking with "UART device as the abstraction level" is reasonable and will offer enough flexibility I don't think we need our own "wired transport" abstraction over the UART driver one.

Thanks for picking this up!

@caksoylar caksoylar added core Core functionality/behavior of ZMK split labels Oct 10, 2023
@kdb424
Copy link

kdb424 commented Oct 16, 2023

I've got great interest in this, and will be tracking and testing, and porting to Lulu and another unnamed board as soon as PIO is working. I have a backlog of 2 wire splits that I'd love to see get full ZMK support. Not sure how much I can contribute on a code front, but more than willing to give feedback and stress test.

@M1cha
Copy link
Author

M1cha commented Oct 17, 2023

@kdb424 do you need PIO on both sides? For now I only implemented PIO on the peripheral side because:

  • My hw is wired in a way that the receiver supports hardware UART(rx only)
  • The PIO driver is (currently) polling only which means that it can impact the whole system because it prevents any other threads from running. That's okay for the peripheral side because there we're just sending and can thus control when we want to send data. Also, the peripheral doesn't have anything else to do anyway because BLE/USB/... are all handled by the central.

@kdb424
Copy link

kdb424 commented Oct 18, 2023

I've got one and two wire RP2040 boards of various pins used. I'm sure I have something around that I can test with, from dev boards, to full keyboards. I'll find something that works with it to test.

@eiis1000
Copy link

eiis1000 commented Nov 3, 2023

I'm not super familiar with the way QMK/ZMK and these communications work, so this is a pretty layman's question, but to what extent are the remaining TODO items core functionality (which to me means "I can put a nice nano v2 on one half and a sea picro on the other half and the keypresses all register") vs nice-to-haves? More specifically, is this ready for beta testing with that core functionality?

@marcusbuffett
Copy link

Also very curious to know what the status of this is. I've got a wired split keyboard with two sea picros, in theory would I be able to use this branch and things would work?

@M1cha
Copy link
Author

M1cha commented Jan 17, 2024

I still intend to clean this up and make this mergeable. I simply got distracted because I'm currently in my keyboard exploration phase to find the best one 😅

Assuming you configure your devicetree correctly, this should work for you as is. I've tested this on two (custom) keyboards so far.

@Timoyoungster
Copy link
Contributor

Timoyoungster commented Jan 17, 2024

I still intend to clean this up and make this mergeable.

Do you have some sort of estimate when you will probably get to it?
I'd also gladly help, if I can do something (do not have much experience with open-source yet but would love to get into it).

@lesshonor
Copy link
Contributor

Not sure how this and #2080 interact, but worth noting.

@M1cha
Copy link
Author

M1cha commented Jan 17, 2024

Not sure how this and #2080 interact, but worth noting.

Thanks for mentioning this. It sounds like this needs to be coordinated better (by the ZMK maintainers?) so we'll work together instead of re-implementing the same thing over and over again and dumping WIP PRs in here.

That being said, from reading the description(I didn't read the code, yet) it looks like it supports more usecases.

@jesseleite
Copy link

Excited about wired split support! For the layman who doesn't fully understand UART, what types of cables could be used to connect peripheral to central? TRRS? USB-C? RJ45?

@nathaniel-brough
Copy link

@M1cha I'd like to give this PR a go. Obviously as it's in it's early stages it's lacking some docs. Is your corne-keyboard repo the best example of this in action?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core functionality/behavior of ZMK split
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants